move late imports to top #26750

xcz011 · 2019-06-09T01:09:33Z

closes PLOT: Fix imports of plotting #26748
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

jreback · 2019-06-09T01:12:25Z

pandas/plotting/_matplotlib/boxplot.py

 import numpy as np

 from pandas.core.dtypes.generic import ABCSeries
 from pandas.core.dtypes.missing import remove_na_arraylike
+from pandas.core.reshape.concat import concat


import from pandas

Thanks! Just updated it.

@jreback I still pretty new here. Can I ask why this place could directly use import from pandas not import as previously ?

previously before @datapythonista reorg (just merged)
the plotting code was basically imported during pandas init
and the pandas namespace was not fully defined yet

it’s now essentially decoupled from that so that it a lazy import
this we can use top of module imports rather than import into each function

Appreciated for this detail explanation. This is helpful.

jreback · 2019-06-09T04:57:50Z

pandas/plotting/_matplotlib/boxplot.py

 import numpy as np

 from pandas.core.dtypes.generic import ABCSeries
 from pandas.core.dtypes.missing import remove_na_arraylike

+from pandas import concat
+from pandas.core.series import Series


Series as well

datapythonista · 2019-06-09T09:10:32Z

@xcz011 probably worth just simply import pandas as pd at the beginning and then just prefix pd.Series and pd.concat. Also, I think you're doing it, but just in case, you'll have to run isort <the modified file> as the imports must be in a specific order.

Did you check if in the other files inside pandas/plotting/_matplotlib are there similar cases?

Thanks!

xcz011 · 2019-06-09T12:49:30Z

@xcz011 probably worth just simply import pandas as pd at the beginning and then just prefix pd.Series and pd.concat. Also, I think you're doing it, but just in case, you'll have to run isort <the modified file> as the imports must be in a specific order.

Did you check if in the other files inside pandas/plotting/_matplotlib are there similar cases?

Thanks!

@datapythonista I found some lazy matplotlib imports in other files , such as pandas/plotting/_matplotlib/hist.py , do I need change those ? Thanks

codecov · 2019-06-09T12:50:20Z

Codecov Report

Merging #26750 into master will increase coverage by 0.02%.
The diff coverage is 72.22%.

@@            Coverage Diff             @@
##           master   #26750      +/-   ##
==========================================
+ Coverage    91.7%   91.72%   +0.02%     
==========================================
  Files         179      178       -1     
  Lines       50767    50774       +7     
==========================================
+ Hits        46555    46574      +19     
+ Misses       4212     4200      -12

Flag	Coverage Δ
#multiple	`90.32% <72.22%> (+0.03%)`	⬆️
#single	`41.19% <0%> (-0.1%)`	⬇️

Impacted Files	Coverage Δ
pandas/plotting/_matplotlib/tools.py	`78.4% <100%> (ø)`	⬆️
pandas/plotting/_matplotlib/timeseries.py	`66.32% <100%> (ø)`	⬆️
pandas/plotting/_matplotlib/style.py	`85.96% <100%> (ø)`	⬆️
pandas/plotting/_matplotlib/misc.py	`38.93% <50%> (+0.9%)`	⬆️
pandas/plotting/_matplotlib/boxplot.py	`75.59% <66.66%> (+0.48%)`	⬆️
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/indexes/interval.py	`96.11% <0%> (-0.32%)`	⬇️
pandas/core/frame.py	`96.88% <0%> (-0.12%)`	⬇️
pandas/core/indexes/datetimes.py	`96.37% <0%> (ø)`	⬆️
pandas/core/indexes/timedeltas.py	`90.96% <0%> (ø)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7748ca...d95a9bf. Read the comment docs.

codecov · 2019-06-09T12:50:20Z

Codecov Report

Merging #26750 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26750      +/-   ##
==========================================
- Coverage    91.7%   91.69%   -0.01%     
==========================================
  Files         179      179              
  Lines       50767    50764       -3     
==========================================
- Hits        46555    46550       -5     
- Misses       4212     4214       +2

Flag	Coverage Δ
#multiple	`90.29% <100%> (ø)`	⬆️
#single	`41.2% <0%> (-0.09%)`	⬇️

Impacted Files	Coverage Δ
pandas/plotting/_matplotlib/boxplot.py	`75.71% <100%> (+0.59%)`	⬆️
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/frame.py	`96.88% <0%> (-0.12%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7748ca...256a536. Read the comment docs.

datapythonista · 2019-06-09T20:46:32Z

@xcz011 all those were lazy to avoid requiring matplotlib if plotting wasn't used. Now that is managed in pandas/plotting/_core.py::_get_plot_backend that doesn't load pandas/plotting/_matplotlib until the user plots something.

So, you can move the other imports to the top too, that's not required and should improve readability of the file.

xcz011 · 2019-06-10T03:06:26Z

@xcz011 all those were lazy to avoid requiring matplotlib if plotting wasn't used. Now that is managed in pandas/plotting/_core.py::_get_plot_backend that doesn't load pandas/plotting/_matplotlib until the user plots something.

So, you can move the other imports to the top too, that's not required and should improve readability of the file.

@datapythonista Thanks for this detail explanation! I just made change to most of files in _matplotlib, except core.py. I not fully sure which do I need move those lazy import to the top too ? Thanks !

datapythonista

Looks good, couple of comments. You can also move the ones in _core.py (the pandas and matplotlib ones, you can leave the rest where they are I think).

datapythonista · 2019-06-10T09:03:05Z

pandas/plotting/_matplotlib/hist.py

@@ -2,6 +2,7 @@

 import matplotlib.pyplot as plt
 import numpy as np
+from scipy.stats import gaussian_kde


The scipy ones should be moved. It's the same case as with matplotlib, we don't require it unless you call a function that uses it. In this case, if you call a plot (like the line plot), you'll need matplotlib, but not scipy, which you only need if you call the kde plot.

This is why the tests are failing (not sure if you've seen the CI is in red). Can you restore the scipy ones please?

Got it! Thanks!

datapythonista · 2019-06-10T09:05:08Z

pandas/plotting/_matplotlib/misc.py

@@ -1,6 +1,12 @@
 # being a bit too dynamic
+from math import pi, sqrt


You can delete the comment, I guess it refers to how we were importing before. But doesn't seem to add much value in any case.

@jreback do you want to avoid loading math unless the user calls the andrews_curves plot, like it was? Or you're ok with this? same for random below

change these to be np.pi and np.sqrt; we don't use math generally

don't have to load scipy unless user call kdeplot

jreback · 2019-06-11T00:22:10Z

pandas/plotting/_matplotlib/misc.py

@@ -1,6 +1,12 @@
 # being a bit too dynamic
+from math import pi, sqrt


change these to be np.pi and np.sqrt; we don't use math generally

only import scipy when it is kde and density

jreback · 2019-06-11T18:42:20Z

looks fine, these are isorted by default right? @WillAyd

WillAyd · 2019-06-11T18:45:53Z

isort doesn't make mods by default in our CI but would fail it if out of order, so this looks good

WillAyd · 2019-06-11T18:46:45Z

Thanks @xcz011 !

move late import to top

830cf25

jreback requested changes Jun 9, 2019

View reviewed changes

fix isort check and import concat from pandas

e352331

gfyoung added Clean Visualization plotting labels Jun 9, 2019

jreback reviewed Jun 9, 2019

View reviewed changes

update imports for boxplot

256a536

Move lazy imports to top for rest of _matplotlib

2caafc8

datapythonista reviewed Jun 10, 2019

View reviewed changes

restore scipy import back to function

a0a1d2a

don't have to load scipy unless user call kdeplot

jreback requested changes Jun 11, 2019

View reviewed changes

xcz011 added 3 commits June 10, 2019 20:33

remove outdated comment

fcdc2d5

switch pi sqrt import from math to numpy

c3863d5

restore scipy import back as lazy import

d95a9bf

only import scipy when it is kde and density

jreback approved these changes Jun 11, 2019

View reviewed changes

jreback added this to the 0.25.0 milestone Jun 11, 2019

WillAyd approved these changes Jun 11, 2019

View reviewed changes

WillAyd merged commit deffee6 into pandas-dev:master Jun 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move late imports to top #26750

move late imports to top #26750

xcz011 commented Jun 9, 2019

jreback Jun 9, 2019

xcz011 Jun 9, 2019

xcz011 Jun 9, 2019

jreback Jun 9, 2019

xcz011 Jun 10, 2019

jreback Jun 9, 2019

datapythonista commented Jun 9, 2019

xcz011 commented Jun 9, 2019 •

edited

Loading

codecov bot commented Jun 9, 2019 •

edited

Loading

codecov bot commented Jun 9, 2019

datapythonista commented Jun 9, 2019

xcz011 commented Jun 10, 2019 •

edited

Loading

datapythonista left a comment

datapythonista Jun 10, 2019

xcz011 Jun 10, 2019

datapythonista Jun 10, 2019

jreback Jun 11, 2019

jreback Jun 11, 2019

jreback commented Jun 11, 2019

WillAyd commented Jun 11, 2019 •

edited

Loading

WillAyd commented Jun 11, 2019

		@@ -1,6 +1,12 @@
		# being a bit too dynamic
		from math import pi, sqrt

move late imports to top #26750

move late imports to top #26750

Conversation

xcz011 commented Jun 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista commented Jun 9, 2019

xcz011 commented Jun 9, 2019 • edited Loading

codecov bot commented Jun 9, 2019 • edited Loading

Codecov Report

codecov bot commented Jun 9, 2019

Codecov Report

datapythonista commented Jun 9, 2019

xcz011 commented Jun 10, 2019 • edited Loading

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jun 11, 2019

WillAyd commented Jun 11, 2019 • edited Loading

WillAyd commented Jun 11, 2019

xcz011 commented Jun 9, 2019 •

edited

Loading

codecov bot commented Jun 9, 2019 •

edited

Loading

xcz011 commented Jun 10, 2019 •

edited

Loading

WillAyd commented Jun 11, 2019 •

edited

Loading